[IR] Don't skip catchswitch when calling BB::getFirstNonPHIOrDbgOrAlloca()
#136056
Conversation
`getFirstNonPHIOrDbgOrAlloca()`
`BasicBlock::getFirstNonPHIOrDbgOrAlloca()` is supposed to yield the
first instruction in the block which is not a PHI or dbg or alloca
instruction. To that effect, it also skips an EHPad instruction which
might be present after any PHIs. Normally, the latter is fine, but it
has an unexpected interaction with `catchswitch` blocks:
```
bb:
; optional PHIs
%0 = catchswitch within none [label %catch] unwind to caller
```
If we skip over the `catchswitch` instruction in the previous block,
we'll return the end iterator, despite `catchswitch` not being a PHI or
dbg or alloca instruction.
This patch amends the logic to not skip an EHPad instruction which is
also a terminator. This means that calling
`getFirstNonPHIOrDbgOrAlloca()` on a non-empty block with a terminator
(such as the above) will always yield a valid instruction iterator. This
appears to be the expected behavior anyways, as most usages of this
method do not check that the returned iterator is `end()`.
Closes llvm#136048.
|
@llvm/pr-subscribers-llvm-ir Author: Tyler Lanphear (tylanphear) Changes
If we skip over the This patch amends the logic to not skip an EHPad instruction which is also a terminator. This means that calling Closes #136048. Full diff: https://github.com/llvm/llvm-project/pull/136056.diff 1 Files Affected:
diff --git a/llvm/lib/IR/BasicBlock.cpp b/llvm/lib/IR/BasicBlock.cpp
index c632b1b2dc2ab..f870fb53307ea 100644
--- a/llvm/lib/IR/BasicBlock.cpp
+++ b/llvm/lib/IR/BasicBlock.cpp
@@ -455,7 +455,7 @@ BasicBlock::const_iterator BasicBlock::getFirstNonPHIOrDbgOrAlloca() const {
if (InsertPt == end())
return end();
- if (InsertPt->isEHPad())
+ if (InsertPt->isEHPad() && !InsertPt->isTerminator())
++InsertPt;
if (isEntryBlock()) {
|
tylanphear
changed the title
catchswitch instructions when calling getFirstNonPHIOrDbgOrAlloca()catchswitch when calling BB::getFirstNonPHIOrDbgOrAlloca()
|
Interesting bug! This is exposing a conflict between However changing this is also in conflict with other use cases:
I would not recommend changing this functions behavior to deviate from the documentation for two reasons:
IMO, a cleaner fix would be to change |
|
@majnemer I see. I'm not overly familiar with I think it's possible to make the modification to |
|
Another way to go would be to do something like: if (auto It = BB->getFirstNonPHIOrDbgOrAlloca();
It != BB->end() &&
!CheckForNonVecCallsInSameBlock(&*It,
BB->getTerminator()))
return Res;This is consistent with the existing code as there are no call instructions and thus the condition is vacuously maintained. |
|
That was my first instinct, though I do wonder: should the condition be Probably the other use of |
|
My thinking was that the behavior of the caller should be as-if it just passed in a basic block which just contained a terminator like br. There are no call-sites in such a basic block so I had it continue on rather than return. |
|
The issue in SLP was fixed by @alexey-bataev in 5fe91f1 -- as discussed above, I don't think this PR needs to be pursued any further. Thanks @majnemer for review! |
BasicBlock::getFirstNonPHIOrDbgOrAlloca()is supposed to yield the first instruction in the block which is not a PHI or dbg or alloca instruction. To that effect, it also skips an EHPad instruction which might be present after any PHIs. Normally, the latter is fine, but it has an unexpected interaction withcatchswitchblocks:bb: ; optional PHIs %0 = catchswitch within none [label %catch] unwind to callerIf we skip over the
catchswitchinstruction in the previous block, we'll return the end iterator, despitecatchswitchnot being a PHI or dbg or alloca instruction.This patch amends the logic to not skip an EHPad instruction which is also a terminator. This means that calling
getFirstNonPHIOrDbgOrAlloca()on a non-empty block with a terminator (such as the above) will always yield a valid instruction iterator. This appears to be the expected behavior anyways, as most usages of this method do not check if the returned iterator isend().Closes #136048.